Skip to content

fix(server): close StdioServerTransport when stdin ends or closes - #2494

Open
claude[bot] wants to merge 6 commits into
mainfrom
claude/fix-2002-stdin-eof-close
Open

fix(server): close StdioServerTransport when stdin ends or closes#2494
claude[bot] wants to merge 6 commits into
mainfrom
claude/fix-2002-stdin-eof-close

Conversation

@claude

@claude claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Requested by Felix Weinberger

Before / After

Before: when an MCP client hangs up its end of the stdio pipe — window closed, session restarted, host crashed — the server never notices. StdioServerTransport only listens for data and error on stdin, so stdin EOF is silently ignored: onclose never fires, nothing tears down, and any server holding a keep-alive handle (timer, connection pool, watcher) lingers forever. In practice this means zombie MCP server processes accumulating with every client restart until someone kills them by hand.

After: stdin reaching EOF (or being destroyed) closes the transport and fires onclose exactly once. That propagates through the existing close chain — Protocol._onclose for hand-wired Server/McpServer, and wire.onclose teardown for serveStdio — so a well-behaved server releases its handles and the process exits naturally. This is what the MCP stdio binding asks for: servers "SHOULD exit promptly when their standard input is closed"; stdin EOF is the primary graceful-shutdown signal, and on Windows (where no signal is delivered when the parent goes away) the only reliable one.

How

  • packages/server/src/server/stdio.ts — new _onstdinclose handler (same arrow-function pattern as _onstdouterror) registered for stdin end and close in start(), removed in close(). It calls this.close(), which is idempotent via the existing _closed guard, so onclose fires exactly once even when end and close both fire or close() is also called explicitly. Class JSDoc documents the behavior.
  • packages/server/test/server/stdio.test.ts — four new unit tests: onclose fires on stdin end (using autoDestroy: false, emitClose: false so the end listener is proven independently of close); onclose fires on stdin destroy (close); onclose fires exactly once when close() follows stdin EOF; messages that arrived before EOF are still delivered.
  • test/integration/test/processCleanup.test.ts + new fixture serverWithKeepAlive.ts — end-to-end regression test for the zombie itself: spawns a real SDK server child that holds a keep-alive interval and releases it in onclose, completes an initialize round-trip, closes the child's stdin (no signals), and asserts the process exits with code 0. Verified this test fails against the unfixed transport (child stays alive past the bound and has to be SIGKILLed) and passes with the fix.
  • test/integration/test/__fixtures__/serverThatHangs.ts — the fixture's signal handler now swallows the sendLoggingMessage rejection it hits once the transport closes itself on stdin EOF (previously an unhandled rejection); the fixture intentionally keeps hanging either way, preserving what the signal-escalation test exercises.
  • .changeset/stdio-server-stdin-eof-close.md — patch changeset for @modelcontextprotocol/server.

No opt-out flag. Considered and deliberately omitted: once stdin has ended no further input can ever arrive, so an open transport has nothing left to transport — there is no coherent use case for opting out, the spec words this as server behavior that SHOULD happen, and the community PR #2003 proposing the same two listeners drew no maintainer request for a flag. Servers that need to finish in-flight work can still do so in their onclose handler before letting the process drain; anything more exotic can pass a custom Readable.

Out of scope: the parent-PID watchdog for hosts that die without closing the pipe — that is PR #1712's lane and the broader remainder of #208.

Tests

  • pnpm --filter @modelcontextprotocol/server test — 450 passed (40 files)
  • test/integration processCleanup.test.ts — 4 passed (including the new e2e; confirmed it fails without the fix)
  • pnpm --filter @modelcontextprotocol/server check (typecheck + eslint + prettier) — clean; touched integration files are eslint/prettier clean

Fixes #2002. Also addresses the stdin-EOF part of #208 (self-termination when the host closes the pipe); the periodic parent-PID liveness check discussed there remains open and is intentionally not duplicated here. Credit to #2003, which proposed the same two-listener fix — this PR lands it against current main with unit + end-to-end regression coverage and the fixture cleanup.

Per the MCP stdio binding, servers should exit promptly when their
standard input is closed - stdin EOF is the primary graceful-shutdown
signal, and on Windows (where no signal is delivered when the host
goes away) the only reliable one.

StdioServerTransport previously listened only for 'data' and 'error'
on stdin, so when an MCP client hung up its end of the pipe (window
closed, session restarted, host crashed) the transport never noticed:
onclose never fired, nothing tore down, and server processes
accumulated as zombies until killed by hand.

The transport now attaches 'end' and 'close' listeners on stdin that
close the transport (idempotently - onclose still fires exactly once
if close() is also called), so Server/McpServer and serveStdio tear
down through the existing onclose chain and a well-behaved server
process exits naturally.

The serverThatHangs fixture now swallows the sendLoggingMessage
rejection its signal handler hits once the transport closes itself on
stdin EOF; the fixture keeps hanging on purpose either way.

Fixes #2002
@changeset-bot

changeset-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f9d3eed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@modelcontextprotocol/server Patch
@modelcontextprotocol/core Patch
@modelcontextprotocol/client Patch
@modelcontextprotocol/server-legacy Patch
@modelcontextprotocol/codemod Patch
@modelcontextprotocol/core-internal Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Jul 14, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2494

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2494

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2494

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2494

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2494

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2494

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2494

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2494

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2494

commit: f9d3eed

Comment thread packages/server/src/server/stdio.ts
…rocessMessage

Review nit on #2494: with the transport now closing on stdin EOF, the
two await tryServeListen(message) sites in processMessage were the only
awaits not followed by the isTornDown() re-check, so a buffered final
message racing stdin EOF on a modern-pinned connection dereferenced
state.instance.channel after the state had been reassigned to closed,
surfacing TypeError: Cannot read properties of undefined (reading
'channel') through options.onerror. Guard both sites like every other
await in the function, and add a deterministic regression test (final
'data' chunk with 'end' on the next tick, emitted from a macrotask so
the EOF handler beats the pump's microtask continuation, as it does
with real stream events).

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8

@claude claude Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tryServeListen race I flagged last run is fixed in f98dd15 (both isTornDown() re-checks + regression test) — thanks. Beyond the new inline EPIPE finding, this run also examined and ruled out two candidates: that the stdin-EOF close aborts in-flight requests before their responses are written, and that responses to requests received before EOF are silently dropped — verifiers refuted both (pre-EOF messages are fully delivered and answered; the existing unit test covers this).

Extended reasoning...

A new bug was found this run (post-close EPIPE crash via the removed stdout error listener), so approval is off the table and the inline comment carries that finding. This note only records progress and coverage: the previous run's serveStdio race was addressed in f98dd15 with the exact fix suggested plus a regression test, and two finder candidates about dropped/aborted in-flight responses at stdin EOF were adversarially examined and refuted, so they need not be re-explored.

Comment thread packages/server/src/server/stdio.ts Outdated
close() removed the transport-level stdout 'error' listener, but a write
accepted before close (write() returned true, per-send listener already
detached) can still fail asynchronously at the OS level — e.g. EPIPE when
the client that hung up also stops reading our stdout. With the new
stdin-EOF close, that left stdout listener-less at exactly the moment a
late EPIPE is most likely, turning it into an unhandled 'error' event
that crashes the process with exit code 1 instead of the clean exit this
change promises.

Leave a swallow-only 'error' listener attached during close() so a
post-close stream error is absorbed, and add a regression test that
fails without the fix.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8

@claude claude Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the new inline findings, I confirmed that both issues flagged in my earlier review rounds are fixed at head 049610c: processMessage now re-checks isTornDown() after both await tryServeListen(message) sites (with a regression test for the buffered-final-message/EOF race), and close() now leaves a swallow-only stdout 'error' listener so a late EPIPE from a pre-close pending write can no longer crash the process (also covered by a new test).

Extended reasoning...

Bugs were found this run and are posted as inline comments, so this is not an approval — the response-drop finding on stdin EOF is substantive enough to warrant human judgment on drain-vs-immediate-close semantics. This note only records what was re-examined and ruled out: the two defects flagged in prior review passes (the serveStdio teardown race after tryServeListen, and the unhandled post-close EPIPE on stdout) were verified as fixed in the current diff at 049610c, each with accompanying regression tests, so those threads need not be re-explored.

Comment thread packages/server/src/server/stdio.ts Outdated
Comment on lines 73 to 88
// with no listener attached, that late 'error' event would crash the
// process with an uncaught exception.
};
_onstdinclose = () => {
// stdin reaching EOF (or being destroyed) means the client has hung up and no
// further input can ever arrive. The MCP stdio binding says servers should exit
// promptly when their standard input closes — this is the primary graceful
// shutdown signal, and on some platforms (e.g. Windows) the only reliable one.
// Close the transport so `onclose` fires and the process can exit naturally.
this.close().catch(() => {
// Ignore errors during close — nothing more can be read anyway
});
};

/**
* Starts listening for messages on `stdin`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Closing the transport immediately on stdin end aborts every in-flight request handler (via Protocol._onclose) and flips _closed so their responses can never be written — but on a half-closed pipe the client is still reading stdout, so requests whose bytes arrived before EOF (the classic cat requests.jsonl | mcp-server > responses.jsonl batch idiom, or any client that pipelines its final request and half-closes) now silently lose their responses. This is a regression: an A/B run at PR head shows the final response is delivered with these two listeners detached and dropped with them attached, deterministically for any handler that awaits anything. Consider a bounded drain on end — stop reading, let requests delivered before EOF settle (the pending-request tracking in serveStdio's channel, whenRequestsAnswered, is exactly this shape), then close.

Extended reasoning...

The bug

stdin end/close now trigger _onstdinclosethis.close() synchronously (packages/server/src/server/stdio.ts:76-85, registered at :100-101). close() flips _closed — so every subsequent send() rejects with StdioServerTransport is closed — and fires onclose, which Protocol._onclose (packages/core-internal/src/shared/protocol.ts:825-854) turns into an abort of every in-flight request handler's AbortController with SdkError: Connection closed. A handler that completes after abort also returns without sending its response (the post-abort check around protocol.ts:1102-1105).

But stdin EOF only closes the read direction. On a half-closed pipe the client is still reading the server's stdout, which remains fully writable — and responses to requests whose bytes arrived before EOF are still owed. The PR hard-closes the write side the instant the read side ends.

Step-by-step proof

  1. A client pipelines its final request and half-closes — e.g. cat requests.jsonl | mcp-server > responses.jsonl, a one-shot smoke test, or any scripted client that writes tools/call (id 2), calls child.stdin.end(), and keeps reading stdout to EOF.
  2. Node emits 'data' with the final line; _ondataonmessageProtocol._onrequest starts the tool handler. The handler hits its first real await and suspends; its continuation is a promise microtask.
  3. The stream emits 'end' from the process.nextTick queue right behind that last 'data' — and nextTick drains before promise microtasks. So _onstdincloseclose() runs while the handler is still in flight. This ordering is deterministic, not a rare race: any handler that awaits anything (a timer, I/O, an LLM call — i.e. every real tool) loses.
  4. Protocol._onclose aborts the handler's signal (ConnectionClosed). Whether the handler bails on the abort or runs to completion, its response is never written: the post-abort guard suppresses it, and even without that, transport.send() now rejects because _closed is set.
  5. The client, reading stdout until EOF per normal half-close semantics, never receives the id:2 response for a request the server accepted.

This was verified empirically at PR head (049610c) with real spawned pipes and the real McpServer + StdioServerTransport: with the PR's listeners attached, a pipelined tools/call whose handler awaits 150ms observes ctx.mcpReq.signal.aborted === true and its response never reaches the parent; a control run identical except with the two 'end'/'close' registrations detached (pre-PR behavior) delivers the response. So this is a regression opened by this PR for a pattern that previously worked, not a pre-existing limitation.

Why the PR's own reasoning and test don't cover this

  • The PR's no-opt-out rationale — 'once stdin has ended an open transport has nothing left to transport' — overlooks the write direction: it still has the responses to already-delivered requests to transport. The suggested escape hatch ('servers can finish in-flight work in their onclose handler') cannot work for responses, because by the time user onclose code runs, send() already rejects and the handlers have already been aborted.
  • The unit test 'should still deliver messages that arrived before stdin ended' asserts delivery to onmessage only — the response round-trip is exactly the part that breaks. Arguably the current behavior is the worst combination: the pre-EOF request's side effects run (or start running), while its response is guaranteed to be dropped.
  • Notably, this PR itself holds the opposite invariant one layer up: serveStdio's probe-discard path is documented as 'a probe request the entry accepted must never go silently unanswered' and implements bounded drain machinery for it (StdioConnectionChannel._pendingRequests / whenRequestsAnswered / DISCARD_ANSWER_TIMEOUT_MS in packages/server/src/server/serveStdio.ts). The transport-level EOF close violates that same invariant for every pre-EOF request.

Addressing the refutation (spec + cross-SDK)

One verifier argued this is intentional, spec-anchored behavior. Two responses:

  • Spec: the stdio binding says servers 'SHOULD exit promptly when their standard input is closed', and the lifecycle doc has the client initiate shutdown by closing stdin. Neither licenses discarding responses to requests the server already accepted — a bounded drain-then-exit (flush answers to pre-EOF requests, then close) is equally 'prompt' and is the universal Unix filter/half-close convention. The shutdown sequence ('close stdin, then wait for the server to exit') is in fact more compatible with drain-then-exit than with abort-and-drop.
  • python-sdk parity: the refutation claims python-sdk also cancels in-flight handlers on stdin EOF; another verifier traced the opposite (anyio task group waits for in-flight handler tasks while the stdout writer stays open). The two readings conflict and I could not independently resolve them here, so I don't rest the finding on parity in either direction. What is not in dispute is the TS-side A/B result: within this SDK, this PR converts a working response-delivery path into a silent drop.

Impact and fix

Any batch/one-shot stdio usage — shell pipelines, CI smoke tests, scripted clients that pipeline their last request before half-closing — silently loses the final response(s) while the request's side effects still execute. The failure is silent on both sides: the server sees only an aborted signal and a swallowed send() rejection; the client just sees stdout EOF without the answer.

Fix shape: on stdin 'end', stop reading and begin a graceful drain instead of an immediate hard close — defer close() (and hence onclose/handler abort) until requests delivered before EOF have been answered, bounded by a timeout as a backstop, mirroring what whenRequestsAnswered already does in this PR's serveStdio channel. Alternatively, at minimum document that half-closed batch usage is unsupported — but given the pattern worked before this PR, draining is the better outcome.

Comment thread packages/server/src/server/stdio.ts Outdated
…t lifecycles

Review nit on #2494: the swallow-only stdout 'error' listener attached in
close() was per-instance and never removed, and _stdout defaults to the
process-global process.stdout — so repeated create->close transport cycles
in one process piled up listeners (MaxListenersExceededWarning after 11
cycles).

Drop the extra listener entirely: make _onstdouterror a no-op once the
transport is closed and keep it attached through close(), so a late write
failure (e.g. EPIPE from a write accepted before close) is still swallowed
with no second listener needed. Tag the kept listener so the next transport
to start on the same stream sweeps stale ones away — the listener count on
a shared stream stays bounded at one per lifecycle instead of growing
without limit. Add a regression test covering repeated cycles on shared
streams.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8
Comment thread packages/server/src/server/stdio.ts
Comment thread packages/server/src/server/stdio.ts
start() registered stdin 'end'/'close' listeners but never checked whether
the stream had already ended or been destroyed before start() ran. Node
emits those events once, so a custom stream (e.g. a net.Socket the peer
reset during async setup) handed the transport a dead stream that never
fired onclose — a zombie connection. Now start() detects the already-dead
stream via readableEnded/destroyed and defers _onstdinclose in a microtask.

Also document in the stdio serving guide that stdin EOF tears the
connection down automatically (no signal handler needed) and that servers
holding their own keep-alive handles should release them on close, with a
synced snippet showing the onclose-release pattern.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016gag9FFr6udcsF8qvMvMV8

@claude claude Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

This review may be incomplete: some analysis steps could not run due to a temporary API capacity limit.

The four follow-up commits check out — I verified each addresses its corresponding finding from the earlier rounds, and I found no new issues at the current head (f9d3eed). Deferring to a human because the half-close question from the earlier round is still open in the code: _onstdinclose still closes the transport immediately on stdin end, so the in-flight-response drain tradeoff flagged inline remains a design call for a maintainer.

What was reviewed on this revision:

  • The isTornDown() re-checks after both await tryServeListen sites in serveStdio.ts, plus the new race test — correct and matching the function's existing convention.
  • The post-close stdout 'error' swallow (_onstdouterror no-op once _closed, kept attached) and the symbol-tagged sweep in start() — checked the cross-instance edge (a still-open transport's untagged listener is never swept; a stale tagged listener left behind is a harmless no-op until the next start()).
  • The readableEnded || destroyed pre-dead-stream check with its microtask deferral, and the two new unit tests covering ended-not-destroyed and destroyed-before-start shapes.
  • The docs/serving/stdio.md keep-alive release section against the actual server.server.onclose behavior — consistent.
Extended reasoning...

Overview

This PR makes StdioServerTransport close itself (firing onclose) when stdin ends or closes, so stdio servers stop lingering as zombies after a client hangup. It touches the transport (packages/server/src/server/stdio.ts), the serveStdio entry's message pump, docs, examples, unit tests, and an end-to-end integration test with a new fixture. Since the last review body, four commits landed (f98dd15, 049610c, a8b799d, f9d3eed), each addressing one finding from the earlier review rounds; I verified each fix in the current head and found no new issues in this run.

Security risks

None identified. The change is transport lifecycle logic on local stdio pipes — no auth, parsing of untrusted structure, or network surface is affected. The main risk class is availability (process crash / premature teardown), which is exactly what the earlier rounds probed: the unhandled-EPIPE crash is now fixed by the retained no-op error listener, and the listener-accumulation leak is fixed by the tagged sweep.

Level of scrutiny

High. This is a behavior change to the default lifecycle of every stdio MCP server built on the SDK — after this PR, every client hangup triggers transport close and Protocol._onclose, a path that previously only ran on explicit close() or stdout error. The earlier rounds showed this exposes real ordering hazards (nextTick 'end' vs. microtask continuations), and one flagged consequence remains unaddressed in code: closing immediately on stdin end aborts in-flight handlers and drops responses to requests that arrived before EOF, regressing the half-close batch idiom (cat requests.jsonl | server > responses.jsonl). Whether to drain pending responses before closing (the inline comment points at whenRequestsAnswered as an existing shape for this) versus closing promptly per the spec's SHOULD is a design tradeoff the maintainers should decide — it is not something I can rule correct either way, so approval is not appropriate even though the implementation of what the PR does attempt is now solid.

Other factors

Test coverage is strong: each prior finding gained a dedicated regression test (the serveStdio race test, the post-close EPIPE swallow test, the create→close cycle listener-count test, the two pre-dead-stream tests), plus the e2e zombie test that was verified to fail pre-fix. The docs gap flagged earlier is closed with the keep-alive release section synced from a typechecked example. The remaining open item is the unresolved inline 🔴 on the half-close/drain question, with no author response yet — that outstanding thread is the primary reason to keep a human in the loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: StdioServerTransport doesn't handle stdin close/end — causes zombie process accumulation

2 participants